Restore #185: verify a Cloudinary media url is our asset - #188
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c99e86cdc5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| !parsed.pathname.startsWith(`/${cloudName}/`) || | ||
| !parsed.pathname.includes(`/${CLOUDINARY_FOLDER}/`) |
There was a problem hiding this comment.
A client can still bypass the upload pipeline with a Cloudinary remote-fetch URL such as https://res.cloudinary.com/<our-cloud>/image/fetch/https://attacker.example/petnote/image.jpg: its pathname starts with our cloud name and contains /petnote/, so every affected media-writing callable accepts it, while Cloudinary serves bytes fetched from an attacker-controlled origin. Validate the Cloudinary resource/delivery segments (for example, require the expected image|video/upload form) and locate the folder in the public-ID portion rather than anywhere in the pathname.
Useful? React with 👍 / 👎.
Reverts the revert in
72c8efe. That rollback was made on the wrong diagnosis — #185 was blamed for the upload outage, and it never touchedmedia.tsor the client upload path. The actual cause wasmax_file_sizein the signed parameter set, fixed in #187.#185 fixes a real vulnerability, so dropping it was a loss with no compensating gain.
What comes back
validateTrustedHttpsUrlchecked protocol and hostname and nothing else.TRUSTED_MEDIA_URL_HOSTSis["res.cloudinary.com"]— which proves the bytes are served by Cloudinary, not that they are ours. Anyone can register a free Cloudinary account, sohttps://res.cloudinary.com/<their-cloud>/...passed unchanged, letting a client:Restored: the cloud-name +
petnote/folder check, the elevensecrets: [CLOUDINARY_CLOUD_NAME]bindings, andmedia-url-ownership.test.ts(7 tests, 4 of which fail without the fix).Verified present after the revert-of-revert:
assertOwnCloudinaryAssetandCLOUDINARY_HOSTinshared.ts, 11 secret bindings acrosspets.ts(2),meetups.ts(2),places.ts(4),users.ts(2),posts.ts(1), and the test file. Localbuild/lint/typecheck:testclean.Sequencing
Deliberately a separate PR from #187, and deliberately merged straight after it, so one deploy carries both and one real upload test covers both. #187 makes uploads work; this makes the resulting url actually get checked.
Note
#185's own PR flagged that the eleven bindings were derived by grepping every call path, not by testing — the emulator suite drives handlers through
.run(), which bypasses secret mounting, so a missed binding passes CI and throwsinternalin production. That caveat still stands and is the reason the post-deploy upload test matters. Task 4 removes this whole failure mode by making the cloud name a plain constant rather than a secret; that lands next.🤖 Generated with Claude Code